Prevent memory leak(s) from QuizChecker. #2656
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2656 +/- ##
=========================================
- Coverage 2.75% 2.75% -0.01%
=========================================
Files 260 260
Lines 12430 12432 +2
Branches 1125 1125
=========================================
Hits 343 343
- Misses 12060 12062 +2
Partials 27 27
Continue to review full report at Codecov.
|
| @@ -37,7 +34,6 @@ | |||
| private boolean isUploadCountFetched; | |||
|
|
|||
| private CompositeDisposable compositeDisposable = new CompositeDisposable(); | |||
| public Context context; | |||
There was a problem hiding this comment.
Here application context is being injected by dagger. @dbrant Is having an instance of application context also considered dangerous?
In this particular class, I agree that storing the context isn't required and activity context can be used everywhere.
There was a problem hiding this comment.
Here application context is being injected by dagger.
LeakCanary was showing a leak that pointed to the QuizChecker class, and when I looked at it, my eyes automatically jumped to the instance of Context.
But in fact the leak was caused by failing to dispose your CompoundDisposable, which this PR also fixes.
Technically, storing a reference to the application context isn't so bad. However, since Android (in its wisdom) doesn't differentiate between application and activity contexts, it's simply too dangerous to do this. There are almost always ways of accomplishing what you want without keeping a reference to a Context.
There was a problem hiding this comment.
Okay, thanks for explaining. Will avoid storing a reference of the context as much as possible. :)
|
Whoops, yes, I remember this advice being mentioned at the 2017 hackathon, lol. Seems like we missed this issue when we code reviewed the PR. I'll add a note to our wiki so hopefully it doesn't happen again. |
Here's a general guideline for designing singleton classes:
Contextas a member variable in your class. Even if you're absolutely sure that you will clear out the Context when you're done, you will forget, and it will cause a memory leak because you will give it an instance ofActivityby mistake.Anyway, in this case it wasn't even necessary to keep an instance of Context. Also, this class maintains an instance of
CompoundDisposablethat keeps the results of Rx observables, which is great, but it never gets disposed! I hooked up the correct disposal mechanism.